fix(schedules): automatic recalculation of derived figures during data entry (#291) - #344
Conversation
Legacy recalculated every dependent read-only figure -- subtotals, net, totals, $/m3 -- when focus left an entry field, before Save. Schedule 2 computed them server-side only, so nothing moved until the Save echo returned. Spine AD-5 was amended 2026-08-20 to permit a display-only mirror: the server stays the sole authority for every stored and returned figure, a mirrored value is never sent on a write, and the Save echo supersedes the mirror. First of the four schedules named in #291. No backend change: every carried input the formulas need is already in the GET document (purchasedWoodOverhead carries the Sch3 PO&P volume/cost, totalCompanyLogging the crown volume and total logging cost), an equivalence Schedule2ServiceTest already asserts. utils/derivedMath.ts (new) The shared primitives, transcribed from the services they mirror: perUnitOf (scale-4 HALF_UP, null when either operand is null or the volume is zero), wholeDollars (scale-0 HALF_UP), addN/subN (CoreUtil null propagation -- null only when both operands are null for addition, asymmetric for subtraction), sumAsZero for the figures legacy never showed blank, and halfUp underneath. halfUp rounds half AWAY FROM ZERO, not Math.round or toFixed: both round toward positive infinity and so disagree with the backend on negatives (Math.round(-1.5) is -1, HALF_UP gives -2). Negatives are reachable from ordinary entry -- netPurchased is a subtraction, so selling more volume than was purchased produces one. It also returns +0 for a small negative that rounds to zero, since -0 renders as "-0" through toLocaleString. hooks/useCommittedValues.ts (new) The blur-committed snapshot. Legacy recalculated on the field's AJAX change handler, not per keystroke, so mirroring per keystroke would churn the derived cells mid-number (1 -> 12 -> 123, each briefly a wrong total). `form` keeps tracking every keystroke because it drives the inputs; `committed` advances only on blur, and re-seeds whenever `data` is replaced -- load, Save echo, Delete reload. components/schedule2/derived.ts (new) Schedule 2's mirror, transcribed line-for-line from Schedule2Service and deliberately NOT returning purchasedWoodOverhead or totalCompanyLogging: both are wholly carried from Schedules 3 and 1, so the page renders them from the document and they cannot drift by construction. components/schedule2/index.tsx Rows render from `figures` -- the mirror while editable, the document as-is outside Draft and in view mode, so the read-only path is untouched. onBlur commits the field; CommaNumberInput needed no change, since it already spreads onBlur onto Carbon's TextInput. Tests: 39 new (21 derivedMath, 13 derived, 5 page). The unit expectations are transcribed from Schedule2ServiceTest so a drift between mirror and server fails a test here rather than surfacing as a figure that jumps on Save; the page tests assert that typing alone moves nothing, that blur moves everything dependent, that the carried rows never move, that the post-Save figures are identical to the pre-Save mirrored ones (AC5), and that view mode renders a deliberately inconsistent stored Subtotal untouched (AC7). Also corrects the Schedule2Response header comment, whose "never recompute them client-side" instruction is what forbade this fix. Verified: full frontend suite 1102/1102 green (62 files), eslint clean, tsc no errors in src/, vite build green. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Second of the four schedules in #291. The Add/Edit Location panel captured each category's $/m3 from the server when it opened (panelPerUnit) and re-seeded it only on open, so the column was stale for the whole of data entry -- and blank outright in COPY mode, where the amounts are cloned but no server figure exists for them yet. Both follow from the same missing mirror; both are fixed here. The panel also never refreshed that column after its own Save (the code deliberately does not re-seed the form, since it already holds the saved values), so a saved edit left the old rate on screen until the panel was reopened. Driving the column from the committed values fixes that too. components/schedule4/derived.ts (new) Mirrors Schedule4Service's per-category perUnit -- cost / volume, null when either operand is null or the volume is zero -- over all twelve categories. Parses with toNum, the same parser buildRequest uses, so the rate on screen is computed from exactly the numbers a Save would send. components/schedule4/index.tsx panelCommitted holds the blur-committed copy of the category grid and feeds the mirror; panelCategories still tracks every keystroke because it drives the inputs. Committed at each panel-open site and at save dispatch, since what is being sent is committed by definition -- which also covers a Save clicked from a field whose blur has not landed. View mode keeps rendering the server's own figures (AC7), so the read-only path is untouched. onCommit is threaded through CategoryRow to CategoryCell, which passes it to CommaNumberInput as onBlur. This page keeps its own committed state rather than the useCommittedValues hook added for Schedule 2: the panel's form is a nested CategoryForm seeded explicitly at four call sites, not the flat FieldValues the hook re-seeds off a document identity, so the hook would not have fit without contorting it. Tests: 13 new (8 derived, 5 page) -- typing alone leaves the rate alone while blur recalculates it, a cleared or zero volume blanks the rate instead of showing Infinity, only the edited category moves, Copy shows the cloned amounts' rate immediately, and View renders a deliberately skewed stored perUnit untouched. Also corrects the Schedule4Response header comment, whose "never recomputed here" instruction is what forbade this fix. Verified: schedule4 tests 43/43, eslint clean, tsc no errors in src/. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Third of the four schedules in #291. Every per-unit cell, Total Silviculture, Subtotal Company Logging, Total Company Logging and the Other-Costs $/m3 came straight off the loaded document, so none of them moved until the Save echo returned. IMPORTANT -- $/m3 rounding is NOT uniform across the schedules, and the defect record's claim that it was is wrong. Schedule1Service.perUnit (and Schedule3Service.perUnit) implement the legacy CoreUtil.bigDecimalDivision: divide at scale 10 HALF_UP, THEN setScale(2, HALF_UP). Schedules 2 and 4 divide at scale 4. Schedule 1's own javadoc records scale 2 as the fix for "the earlier Schedule-1 divergence to 4 decimals", so 1/3 are the legacy-faithful pair and 2/4 are the outliers. The two rules disagree exactly at a two-decimal boundary, which is where the display sits, so utils/derivedMath.ts now carries both perUnitOf (scale 4, Schedules 2/4) and perUnitLegacy (scale 10 then 2, Schedules 1/3), each naming the schedules it serves and warning against unifying them. Schedule1Service.perUnit also takes (volume, cost) -- the reverse of Schedule 2's and 4's -- which is why the new module names its arguments. components/schedule1/derived.ts (new) Transcribed from Schedule1Service:505-557. The per-figure null rules are the other trap and are transcribed individually: subtotalCompanyLoggingCost sums with nulls as 0 and is never blank (legacy seeds its Other-Costs term at 0), while totalSilvicultureCost null-propagates so an empty silviculture block reads blank instead of showing a negative admin cost -- the S02 crown-prefill screen. totalCompanyLoggingCost then adds the two, so a blank Total Silviculture leaves the grand total equal to the subtotal. The Schedule 3 pulls (forestMgmtAdminCost, lessSilvAdminCost, schedule3CrownVolume) and the Other-Costs subtotal are consumed as constants: they cannot move while Schedule 1 is edited. The Other-Costs VOLUME is entered on this page though, so that row's $/m3 does move. components/schedule1/index.tsx The inputs already had an onBlur (groupField, which re-groups the display), so commitField now does both -- grouping only adds separators, which toNum strips, so the order is immaterial to the figures. The Other-Costs volume field had no onBlur at all and gains one. Rows render from `derived` while editable and from the document otherwise, leaving the read-only path untouched. Tests: 22 new (16 derived, 6 page). The unit expectations come from Schedule1ServiceTest's derivedTotals_foldInSchedule3AndLineItems and totalSilviculture_blankWhenSilvCostsAbsent fixtures, including one that pins the scale-2 rule against scale-4 (200000/30000 -> 6.67, not 6.6667). The page tests cover typing-moves-nothing vs blur-moves-the-chain, a volume blur moving only its own row, the Other-Costs rate tracking the entered volume and blanking when it is cleared, Total Silviculture's null semantics as costs are entered, no jump on Save, and view mode rendering a deliberately skewed stored subtotal untouched. All 26 pre-existing Schedule 1 page tests still pass unchanged. Verified: schedule1 tests 48/48 (32 page + 16 derived), eslint clean, tsc no errors in src/. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Last of the four schedules in #291, and the most interdependent: every per-line crown, the Scaling PO&P, all four three-column totals, both timber blocks and Total Overhead came straight off the loaded document. components/schedule3/derived.ts (new) Transcribed from Schedule3Service:164-231 plus Schedule3Constants.resolvePop / scalingPop. Three things make this page harder than the others: * Scaling (33)'s PO&P is ITSELF derived -- round0((popTimberVolume / overheadVolume) * scalingHarvest) -- so it moves with three entered fields (its own harvest and BOTH timber volumes) and then cascades into the PO&P subtotal, Total Costs, both timber costs and Total Overhead. It is the one derived value on this page sitting in an entry column. The subtotal sums the RESOLVED pops (the server puts the derived value into popByCode before summing), so it is included, not treated as 0. * includedUnacceptableCosts mixes an item-38 sub-page constant with the ENTERED Annual Rents (29) harvest, and the main-page document carries only their sum. The constant is recovered once as `loaded total - loaded Annual Rents`, which is exact because that is how the server built the sum. Commented at the call site, since a future reader would otherwise "simplify" it back into the bug. * subtotalOtherCosts is owned by the Other-Acceptable sub-resource and cannot move here, so it is consumed as a constant and still rendered from the document. unacceptableCount is mirrored as well. It sits in the SAME ROW as the Included Unacceptable Costs total and moves by the same entered field, so a moving total beside a frozen count would read as a bug. This settles the choice the defect record left open. utils/derivedMath.ts -- halfUp rewritten to round on the DECIMAL EXPANSION A real wrong-figure bug, caught by its own test while wiring this page. The previous implementation scaled by 10^n and rounded, which loses exact halves to binary representation: 3075/5000 is 0.615, which BigDecimal rounds UP to 0.62, but the nearest double is 0.6149999999999999911, so Math.round(0.615 * 100) gave 61 and the cell showed 0.61. Both operands are money and volume figures, so quotients landing exactly on a half-cent are ordinary, not contrived -- this would have shipped as a visible one-cent error. halfUp now expands the value with toFixed and carries on the first dropped digit. All 62 mirror tests across the four schedules still pass with the new rounding. scalingPop deliberately omits the server's scale-15 intermediate rounding: a double already carries ~15-17 significant digits, so rounding to 15 decimal places adds nothing while the x10^15 needed to do it risks exceeding MAX_SAFE_INTEGER. Documented at the function. Tests: 32 new (25 derived, 7 page), pinned to Schedule3ServiceTest's normalLine_crownIsHarvestMinusPop, harvestOnlyLines_popForcedZero, scalingPop_derivedFromTimberVolumeRatio, fullDocument_derivedCascadeMatchesLegacy, unacceptableCount_addsItem38RowsPlusAnnualRents and emptySchedule_subtotalsAreZero. The page tests use a SELF-CONSISTENT fixture rather than the existing schedule3Doc, whose stored derived values do not satisfy the server's own formulas (Scaling PO&P 100 where the ratio gives 375; Subtotal Actual harvest 8850 where the columns sum to 8350). Asserting a mirror against numbers the server would never produce would prove nothing. The old fixture is left alone -- no existing test asserts those cells -- but it is worth correcting separately. All 38 pre-existing Schedule 3 tests still pass unchanged. Verified: schedule3 49/49 (24 page + 25 derived), all four mirrors 62/62, eslint clean, tsc no errors in src/, npm run build green. Full suite 1176/1177 -- the one failure is a 5s-timeout in Schedule8.test.tsx under full parallel load, an untouched file that shares no code with this change and passes 67/67 in isolation. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…AC5 tests (#291) Applies all 14 patches from the 2026-08-21 adversarial code review (four parallel layers, none sharing the author's context). Two of the four HIGH findings were wrong ANSWERS in the shipped arithmetic; one was a wrong CLAIM about the tests. ARITHMETIC — utils/derivedMath.ts rewritten around an exact decimal core The previous implementation rounded in double/string space and diverged from BigDecimal four ways, all verified against exact BigInt models of the Java: * toFixed PRE-ROUNDS, so a near-tie became a false tie: halfUp(0.4999999951, 0) gave 1 where HALF_UP gives 0. Reachable at ordinary magnitudes -- cost 41,956,411 over volume 40 rendered 1,048,910.27 against the server's 1,048,910.28. A one-cent error in a rate cell. * toFixed switches to exponential notation at |x| >= 1e21, which positional string parsing mis-read: halfUp(2.5e21, 4) returned 0.25. * Materialising the scaled digits as a Number exceeded MAX_SAFE_INTEGER on perUnitLegacy's scale-10 step for any quotient over ~1e6, so the "exact integer arithmetic" the function rested on was not exact. * The Math.min(decimals + 8, 100) clamp was silently wrong above decimals 92. Division and rounding now go through a `Dec` (units / 10^scale over BigInt), so BigDecimal.divide(divisor, scale, HALF_UP) is reproduced at every magnitude. A number's toString() is its shortest round-trip decimal -- the same literal the wire carried and Jackson handed to BigDecimal -- so decomposing it that way is faithful to the server's operand rather than an approximation of it. Fixing this surfaced one more: converting 10^23/10^2 back to a number loses precision that 10^21/1 keeps, so decToNumber strips redundant scale first. scalingPop: the server's scale-15 ratio rounding is RESTORED. Omitting it was a documented decision and the documentation was wrong -- the server's answer IS the rounded-ratio answer, and fidelity to the server was the requirement. With PO&P volume 5,000,000, Crown 1,000,000 and harvest 999,999 the server gives 833,332 and the old mirror 833,333, cascading into nine cells. VERIFICATION — the AC5 tests detected nothing, and now do Every editable page renders `derived ? mirror : document`, so the server's derived figures are never rendered in Draft. The old AC5 tests snapshotted the pre-Save render and compared it to the post-Save render -- the mirror against itself. They passed with 999999 in the echo, and both HIGH arithmetic bugs walked through them. They now assert against the echo's own derived fields, and each page gains a load-time "mirror reproduces the served figures" test. Mutation-checked: a $1 mirror divergence now fails 4 Schedule 2 tests. Schedule 1's page fixture carried NO server derived fields, so its assertions were the mirror measured against hand arithmetic in the same file; it now carries the figures Schedule1Service computes for it. Schedule 4 had no AC5 test at all. useCommittedValues had no test file -- deleting its re-seed effect failed nothing; it now has 9. Also pinned: lessSilvAdminPerUnit (139), asserted by neither side, and the -0 crown normalisation. BEHAVIOUR * Invalid and unparseable committed entries hold their last valid value instead of driving the cascade (ruled by Scho after review). Legacy's failed round-trip left the totals alone; committing garbage let an out-of-range volume move nine cells to a state the server cannot produce, and text like "-" silently dropped its line out of every total. * Gating on fieldErrors does NOT cover fractional costs (they are in range), so Schedules 1 and 3 now route derived costs through wholeDollars as Schedule 2's mirror already did -- removing an inconsistency inside the original diff and keeping cents out of columns the wire types as Integer. * Non-finite entries can no longer reach a cell (enteredNum): toNum accepts "Infinity"/"1e999", which rendered as the glyph through fmtNumber. * Schedule 4 re-seeds its panel from the Save echo. It never did, so the mirror kept driving the $/m3 column for the rest of the session, contradicting AD-5's "superseded by the server echo on every Save" in three files. * useLayoutEffect closes a one-frame window where a freshly loaded document could paint totals computed from the previous entry set. * commitField passes the GROUPED string, so committed and form hold the same text and the unchanged-value skip actually fires. E2E -- the deliverable the fix was explicitly told to include e2e/.../defects.md DIV-1 is the QA record that originated this ticket, and it says: "If recalculation-on-blur is restored, happy-path.feature will fail -- update that scenario as part of the fix." The original branch touched no file under e2e/. happy-path.feature's pre-save block now carries the recalculated figures, deliberately identical to its post-save block, so the scenario asserts mirror-vs-server agreement against a real backend instead of pinning the divergence. DIV-1 closed as resolved. Verified: eslint clean, tsc no errors in src/, npm run build green, full suite 1204/1205 -- the one failure is a timeout in Schedule7a.test.tsx under full parallel load, an untouched file sharing no code with this change, which passes 37/37 in isolation. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…291) Extends #291 past its original Schedules 1-4 scope, after an audit of the remaining schedule pages found the same defect on three of them (Scho widening the ticket). Schedules 6 and 7A here; Schedule 5 -- the largest of the three -- is NOT in this commit. The audit checked each page against BOTH its own code and the legacy render targets, since a page only has this defect if legacy recalculated and the modern page does not. Already correct and left alone: 7B and 9 (served-unless-edited, matching legacy), 10 (every derived cell goes through preview*(form)), 8 (live skidding total; its rates footer is summed client-side and it has no inline row edit for legacy's one in-place recalculation to correspond to). NOT this defect: 11 -- its change handlers target only @this, the original-value indicator panels and messages, so legacy did not recalculate during entry either. SCHEDULE 6 -- the row's $ / m3 only perUnitLegacy, since Schedule6Service.perUnit divides at scale 10 then rounds to 2 (the Schedules 1/3 rule, not 2/4's scale 4). This page had no blur plumbing at all, so onBlur was added to the volume and cost inputs with a committed snapshot per form (one add form, one edit row). Deliberately NOT mirrored, both evidence-backed: * the footer totals -- totalVol/totalCos/totalCal appear in ZERO legacy render or update targets, so legacy left them until Save and rendering them from the document is already faithful. Same call as the Schedule 1 Other Costs footer. * rmg -- no id for it exists anywhere in schedule6.xhtml and no handler targets it, and deriving it needs the year-scoped TSA/TFL code caches that have no REST counterpart (the page's pre-existing deviation A). Removes the page's "deviation D" comment, which read "legacy re-derived them live over ajax, which AD-5 forbids re-implementing on the client" -- accurate about legacy, and exactly the AD-5 reading the 2026-08-20 amendment corrects. SCHEDULE 7A -- the four bridge totals totalMaterial/Deliver/Install = ss + abut; grandTotal = sum(sitePlan, those three, approach, afterInstall, other). No division, so no rounding hazard. The existing onGroup blur handler is reused; it now commits from the same setState updater that regroups, so the baseline holds the string the field shows rather than the pre-grouping value (the bug the code review found on Schedules 1/3). The Add panel passed NO totals, so all four read blank while a new bridge was entered -- the same "blank, not stale" shape as Schedule 4's copy mode. Both fixed by the mirror; BridgeFields' `totals` prop is widened to the four-field shape so it accepts either the served bridge or the mirror. utils/derivedMath.ts: new sumN -- null-tolerant n-ary sum, null only when EVERY operand is null (legacy CoreUtil.sumBigDecimalCosts / Schedule7aService.sum / Schedule5Service.sumCosts). Distinct from sumAsZero, which returns 0 for an all-null input; the choice is per figure, and these totals stay blank. KNOWN GAP: neither page has new tests yet. The 271 targeted tests passing are pre-existing ones that do not assert these cells -- the same situation the code review caught on Schedule 1, where a fixture carrying no derived fields made its assertions self-referential. The mirror code for 6 and 7A is therefore unverified and that is the first work on resume, before Schedule 5. Verified: eslint clean, tsc no errors in src/, npm run build green, targeted 271/271, full suite 1204/1205 -- the one failure is the same load-induced timeout in Schedule8.test.tsx seen on earlier runs, an untouched file that passes 67/67 in isolation (re-confirmed). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ry (#291) Third of the three pages the audit found outside #291's original scope, and the largest -- comparable to Schedule 3. Every derived figure rendered from `served`: the per-category $/m3 cells and all four derived rows (Camp Sub-Total, Camp Total, Access Expense Total, Camp and Access), each with volume, cost and rate. Legacy refreshed exactly those on change, via two dynamic render lists (schedule5ExistingCamp.xhtml:110-111): renderIdsCampExpenseCostUpdates = campSubtotalCost campSubtotalCostVolume campTotalCost campTotalCostVolume totalExpenseCost totalExpenseCostVolume renderIdsAccessExpenseCostUpdates = accessExpenseTotalCost accessExpenseTotalCostVolume totalExpenseCost totalExpenseCostVolume 27 change handlers on the existing-camp page, 25 on new-camp. components/schedule5/derived.ts (new) Transcribed from Schedule5Service:255-300. costPerVolume is the LEGACY rule (divide at scale 10, then scale 2) -- the Schedules 1/3/6 rule, not 2/4's scale 4. Sub-Total sums exactly five costs with Recoveries EXCLUDED; Camp Total subtracts Recoveries and is never clamped, so a Recoveries above the sub-total gives a negative; Access Expense Total sums exactly six; Camp-and-Access adds null-tolerantly. sumN not sumAsZero, so a camp with no costs shows blank totals rather than a fabricated 0, and subN's asymmetry is preserved (a null Recoveries passes the sub-total through, a null sub-total stays null regardless). All four derived rows carry the Associated Camp Volume, so that one field moves every one of their rates -- and BR-03 propagates it into all eleven volume-bearing categories, so their rates move with it too. The page already did that propagation in setCampVolume; the mirror now follows it. RECORDED DEVIATION: otherCampExpenses and otherAccessExpenses keep their SERVED rate. Their $/m3 is not cost/volume -- costPerVolumePerTerm divides EACH item-62 /68 sub-page row by the stamped volume, rounds each term to scale 2, and sums those, which is not equal to rounding the summed cost once. Reproducing it needs the individual row costs and the camp document carries only their sum plus a count. Legacy DID refresh these two on a volume change (schedule5ExistingCamp.xhtml:223 renders otherCampExpensesCostVolume), so this is a real parity gap -- taken deliberately, because a stale-but-correct figure beats a live-but-wrong one and a single-division approximation would break the no-jump-on-Save guarantee. Their COSTS are server-owned and consumed as constants, which is correct: the sub-resource is their only writer. Recoveries has no rate at all, ever -- no volume cell and no $/m3 cell exist. components/schedule5/index.tsx A blur-committed snapshot of the whole camp form. The page already had a per-field onBlur but it only marked the field blurred for validation; it now also commits. The commit takes the WHOLE form rather than one field, because BR-03 means one field's blur can legitimately move every rate. View mode passes a null mirror so the served figures render untouched (AC7). Tests: 23 new (17 derived, 6 page). The unit expectations are transcribed from Schedule5ServiceTest's CampSubTotal / campTotal / AccessTotals nests -- including the 1,644,000 five-component sum with Recoveries excluded, the -20,000 unclamped negative, and the 660,000 canary that a collapsed Camp Total would have left at 180,000. The page tests use the existing cedarFlats fixture, which is fully self-consistent, so the load-time assertion is a genuine mirror-vs-server comparison; they also cover typing-moves-nothing vs blur-moves-the-cascade, the camp-volume propagation moving all four totals, Recoveries feeding Camp Total but never the Sub-Total, the two per-term Other rows keeping their served rate, and view mode rendering a deliberately skewed stored Sub-Total untouched. Verified: eslint clean, tsc no errors in src/, schedule5 + utils 270/270. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ndary (#291) Closes half the test debt the 4acedd2 commit recorded: that commit shipped Schedule 6's mirror with no test asserting the cell it changed -- the same shape as the Schedule 1 gap the code review caught, where a fixture carrying no derived fields made every assertion self-referential. derived.test.ts (new, 9 tests) Expectations transcribed from Schedule6ServiceTest: 1000/50000 -> 50.00 and 400/30000 -> 75.00 from the two RMG-derivation tests, plus the zero-volume and absent-volume nulls from zeroOrAbsentVolume_costPerVolumeNull. Also pins the scale-2 rule against scale-4 (200000/30000 -> 6.67, not 6.6667), the exact half-cent case a float multiply-and-round gets wrong (3075/5000 -> 0.62), a negative credit, and non-finite entry being treated as absent. Schedule6.test.tsx (5 page tests) The page fixture is self-consistent (50000/1000 = 50), so the load assertion is a genuine mirror-vs-server comparison. Covers typing-moves-nothing vs blur-recalculates, the volume half as well as the cost half, a cleared volume blanking the rate instead of dividing by zero, and the Add panel showing a rate as soon as both halves are committed (it previously passed a hardcoded blank, so a new record showed nothing until the first save). The fourth test pins the DELIBERATE BOUNDARY: the footer totals must NOT move during entry. totalVol/totalCos/totalCal appear in no legacy render or update target, so leaving them to the Save echo is faithful, and the test asserts the row's own rate moved while the totals region's text is byte-identical. Without it, a future "consistency" change would widen the mirror past legacy silently. Two of my own expectations were wrong and are corrected here rather than papered over: a blank rate renders as '' not an em dash, and 999,999/1,000 is 999.999 which rounds to 1,000.00 at scale 2 -- not 999.999. Verified: schedule6 98/98, eslint clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Closes the other half of the test debt 4acedd2 recorded, so no page in this branch now carries mirror code without a test asserting the cells it changed. derived.test.ts (new, 9 tests) Expectations transcribed from Schedule7aServiceTest: read_mapsBridgeAndDerivesTotals (8,000 / 800 / 1,200 / 12,000, the grand total including Site Plan -- the service javadoc that omits it is wrong, the code wins), totals_nullWhenNoContributingCost (site-plan-only leaves the three pair totals null and the grand total 1,000) and totals_partialNullAddition (a lone present operand passes through). Also pins that a wholly empty bridge shows four BLANK totals rather than 0 -- the sumN-vs-sumAsZero distinction -- that a real zero is preserved and not treated as absent, grouped entry, a negative credit flowing through both levels, and non-finite entry being treated as absent. Schedule7a.test.tsx (5 page tests) The northFork fixture is self-consistent, which lets the first test be a real mirror-vs-server comparison: committing a cost WITHOUT changing it hands the row from the served bridge to the mirror, so the four figures then come from the client and must still equal what the server sent. That is the shape the code review showed was missing from the original AC5 tests, which compared the mirror with itself. Also covers typing-moves-nothing vs blur-recalculates (with the two untouched pair totals asserted as unmoved), clearing both halves blanking that total instead of showing 0, the Add panel showing totals as soon as a cost is committed (it previously passed no totals at all, so all four read blank for the whole of entry), and an untouched row keeping a deliberately skewed served grand total -- no client recomputation until the reporter commits something. Two query problems in my own tests are fixed rather than worked around: the cost-field labels also contain "Material"/"Deliver"/"Install", so the total lookup is scoped to the .schedule-7a__total blocks and matched on the label exactly; and the Add-panel field query needed scoping, since every row renders its own editor. Verified: schedule7a 132/132, eslint clean, tsc no errors in src/, npm run build green, full suite 1255/1256 -- the one failure is the same Schedule8.test.tsx timeout seen on every full run this session, in an untouched file, confirmed passing 67/67 in isolation twice. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…291) A sub-page surface the earlier audit missed, because it trusted the component's own comment instead of the legacy source. Both derived cells rendered from the document: the per-row $/m3 from `served?.costPerVolume` and the `Totals:` footer from `doc.totals`, while the row cost is an entry field. Only the COST is entered. A row has no stored volume -- `stampedVolume` is the camp's item-141 (CAMP) or item-142 (ACCESS) amount and every row displays it (deviation (B)), and SubPageRowRequest carries description and cost only. So the row rate is enteredCost / stampedVolume and the footer moves with the costs. THE TWO PAGES' FOOTERS ARE DIFFERENT SHAPES and deriveSubPageTotals keeps them that way, transcribed from Schedule5Service.subPageTotals:1257-1290: * CAMP sums cost AND volume, and flags "contributed" on a non-null cost OR -- uniquely -- on a null cost when the stamped volume is non-null, so an all-null-cost list yields 0 rather than null. Its volume is n x stampedVolume, and a null stamped volume totals 0 because legacy starts that accumulator at zero. * ACCESS is cost-only, so an all-null-cost list yields null whatever the volume -- after which the volume is overwritten with the SINGLE camp volume, unconditionally, including on an empty list. CORRECTS A WRONG COMMENT in this file, which claimed "Only an ACCESS description validates on change; the Camp grid carries no f:ajax at all and NEITHER page's cost does". Both halves are wrong: the CAMP grid's COST input carries <f:ajax event="change" render="footer"/> (schedule5CampExpenses.xhtml:78), and on ACCESS both the description AND the cost carry render="@this calcCost" (schedule5AccessExpenses.xhtml:63,75). The footer comment about the totals being "server-derived ... nothing is summed here" is updated too. Legacy was asymmetric -- CAMP refreshed the footer but not the row rate it also displays, ACCESS refreshed the row rate but not the footer. Both cells are mirrored on both pages, applying the divergence ruling already made for Schedules 1/3: legacy moved a figure while leaving another derived from the same entry stale, and reproducing that would read as the very bug being fixed. The footer ARITHMETIC stays page-aware, because that difference is real rather than an inconsistency. Tests: 12 new, covering both pages' footer shapes side by side (same rows and volume, different volumes and rates), the CAMP all-null-cost-yields-0 quirk against ACCESS's null, the null-stamped-volume-totals-0 case, the whole-triple null when nothing contributes, the row rate's scale-2 rounding, grouped entry and non-finite entry. Verified: schedule5SubPage 56/56, eslint clean, tsc no errors in src/. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…lur (#291) The last surface the audit found. The Other Acceptable Costs sub-page rendered its Totals footer from the document (`item.value(data)`) while Total $ and PO&P $ are entry fields, so the subtotal only moved on the Save echo. Legacy refreshed it: an existing row's Total $ or PO&P $ change carried `update="otherCrownTabel footerValues"` (schedule3SubtotalOtherCosts.xhtml:74,83) -- the row's Crown cell AND the footer. The row Crown was already live; the footer was not. The shared Schedule3SubPage component gains an OPTIONAL `deriveSummary`, supplied only by the page whose legacy footer actually refreshed. The Included Unacceptable page deliberately omits it: its only handler was `render="cost"`, the field itself, with no derived target, so leaving that footer to the Save echo is faithful. That asymmetry is the point -- one shared component, two different legacy behaviours, and the config decides. The mirror is transcribed from `Schedule3Service.subtotalOtherCosts`: the TOT rows sum into Harvest and their PO&P peers into PO&P, nulls as 0 (so a cleared row contributes 0 rather than blanking the total), and Crown is the difference. A COMMITTED SNAPSHOT keyed by row, re-seeded on document identity, drives it -- the same shape as useCommittedValues but for a keyed record rather than a flat form. The existing per-field onBlur already regrouped the display; it now commits the grouped string too, so the baseline and the field hold the same text. The four new tests earned their keep immediately: the first version of this fix was INERT. It gated the mirror on `editor.editable`, which does not exist -- the editor hook returns `data`, and `editable` lives on the DOCUMENT -- so `mirroredSummary` was always null and the footer never moved. tsc did not catch it and the load-time test passed anyway, because on load the mirror and the served figure agree by construction. Only the blur tests failed, which is exactly the hole the code review found in the original AC5 tests: a load-time or render-to-render assertion cannot tell a working mirror from a disabled one. Tests: 4 new on the self-consistent fixture (rows 800+600 = 1400 harvest, 300+200 = 500 pop, crown 900) -- load-time agreement, typing-moves-nothing vs blur-recalculates, a PO&P blur moving only the PO&P and Crown columns, and a cleared row counting as 0 rather than blanking the footer. Verified: schedule3 surfaces 75/75, eslint clean, tsc no errors in src/. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…supersedes (#291) Applies the HIGH findings from the 2026-08-21 batch-2 code review (four parallel layers; the verification-gap layer proved several by probe in a throwaway worktree). Every one of these is a lesson from the FIRST review that was applied to Schedules 1-4 and not carried to the new pages -- the corrections lived in commit messages and the defect record, nowhere that could catch a new page. 1. WHOLE DOLLARS -- binding fidelity rule 2, missing from all four new mirrors Every write path sends roundCost(parseDecimalInput(...)); every new mirror summed and divided the raw entry. Schedules 1/2/3 already used wholeDollars; these were the only mirrors without it. Five camp costs of 0.5 gave a mirror Sub-Total of 2.5 against the server's 5; two costs of 100.5 were enough for a visible $1 divergence. 2. STRICT PARSE -- the mirrors parsed more permissively than the wire New: utils/derivedMath.ts `committedNum` (parseDecimalInput + finiteness) and `isUnusableStrictEntry`. The mirrors used enteredNum -> toNum, which is documented as accepting "JS-only forms legacy never allowed": '1e3' -> 1000, '.5' -> 0.5, '0x10' -> 16, mis-grouped '12,34' -> 1234, all of which the wire parser rejects as null. So the mirror displayed figures no Save could persist while validation blocked the write. Schedules 1-4 use toNum on BOTH sides, so enteredNum stays correct there and only there -- both helpers are documented with which pages they serve. 3. THE INVALID-ENTRY GATE -- ruled 2026-08-21, applied to none of the five new surfaces. isUnusableEntry was exported and unused by the new code. An out-of-range value drove the whole cascade; '-' or '1.2.3' committed as null and silently dropped its line from the totals. Schedules 6 and 7A now gate every commit on the page's own validator plus the strict parser (commitRate / commitBridge); Schedules 5, 5-sub-page and 3-sub-page follow in the next commit. 4. SCHEDULE 7A: THE SAVE ECHO NEVER SUPERSEDED THE MIRROR (proven) `rowCommitted` was cleared nowhere -- not in resetTransient, not in the save-all onSuccess -- while the totals rendered from `id in rowCommitted ? mirror : bridge`. Probe: blur a cost, Save, and the input reverted to the echoed 5,000 while Material stayed on the client's 10,000, permanently, with all 132 tests green. This is the identical HIGH defect the first review found on Schedule 4, fixed there, reintroduced here. It also broke AC7: rowCommitted survived resetTransient, so a Draft-computed mirror could render onto a no-longer-editable document. 5. SCHEDULE 7A: A NO-OP BLUR FLIPPED AUTHORITY groupRowField wrote rowCommitted unconditionally -- before the `grouped === current[key]` check and even on the `return prev` path -- so tabbing through an untouched cost field handed the row to the mirror. That bypassed this page's OWN AC7 test: the grandTotal:999999 fixture flipped to 12,000 on a tab-through. Legacy fired on `change`; a tab-through is not one. 6. SCHEDULE 7A: IMPURE STATE UPDATERS setAddCommitted/setRowCommitted were dispatched from inside the setAddForm/ setRowForms updaters -- invoked during render, double-invoked under StrictMode, and executed on renders React discards. Both handlers now compute outside the updater, which also removes the bail-out path behind finding 5. Tests: two new lifecycle tests on Schedule 7A, both mutation-verified. The save-then-inspect one FAILS when either rowCommitted clear is removed and passes with them present -- the invariant the whole design rests on, which no test in this batch asserted. The no-op-blur one pins the tab-through against the skewed AC7 fixture. Verified: 515/515 across utils + schedules 5, 5-sub-page, 6, 7A; eslint clean; tsc no errors in src/. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…owId keying (#291) Group 3 of the batch-2 code review fixes. SCHEDULE 5 CAMP PANEL * The two per-term Other rows now BLANK their rate once the camp volume moves (ruled by Scho). BR-03 rewrites those rows' VOLUME cells while their rate is server-owned -- the per-term `costPerVolumePerTerm` formula divides each sub-page row separately and cannot be reproduced from the sum the document carries -- so with the shipped fixture a camp volume of 60,000 left the row reading 60,000 / 24,000 / 0.31, which no arithmetic reconciles. A blank is honest; a figure computed against an off-screen denominator is not. NOTE the comparison that matters is committed-vs-SERVED CAMP volume, not the row's own volume. My first attempt compared against `served.volume` and blanked the rate on LOAD, because an Other row's stored volume is its item-141/142 amount and legitimately differs from the camp volume. The existing test caught it immediately. * AC7 now gates on the DOCUMENT's editability as well as the panel mode. Gating on `panelMode` alone left a live mirror over a schedule the server would refuse to write: `applySaved` re-seats the panel in edit mode from the echo without consulting its `editable` flag. This also brings the page in line with the Schedule 3 sub-page, which already gated correctly -- two different AC7 gates had landed in one PR. * The invalid-entry gate (ruled 2026-08-21) is applied: `commitEntry` advances the baseline only when the form validates and every entry survives the strict wire parser. An out-of-range camp volume previously drove thirteen cells to a state the server rejects, and a negative Recoveries inflated the displayed Camp Total while its own field was red. SCHEDULE 5 SUB-PAGES * THE JUSTIFICATION IS RE-GROUNDED, and it is now the honest one. The module note claimed legacy refreshed the CAMP footer (`render="footer"`) and the ACCESS row rate (`render="@this calcCost"`). Both citations are wrong: `id="footer"` exists only in schedule8AdditionsAndDeductions.xhtml:282 and `id="calcCost"` exists NOWHERE in the webapp, so both render targets are dangling and legacy refreshed neither cell on either page. Mirroring them is a deliberate improvement, ruled by Scho, recorded as a divergence -- not parity. * Delete no longer desynchronises the two lists (proven by probe). The delete `onSuccess` re-applied surviving drafts to `rows` only, while `applyDocument` had already reset `committedRows` from the echo -- so an edited cost stayed in the input while the footer and every rate reverted, until the next blur. Both lists are now written from one merged expression. * The committed snapshot is keyed by `rowId`, not array index, and the `?? row` fallback is gone. Index pairing against an rowId-keyed list mispairs the moment the arrays differ in length or order, and falling back to the LIVE row reintroduced per-keystroke churn for that one row. * ONE binding for the stamped volume. Three spellings of it were in this table -- the volume cell, the row rate and the footer each resolved it differently -- where the service derives all of them from a single `stampedVolume`. * The commit gate, and the stale "Nothing is computed here" comment corrected. TESTS Schedule 5: two new page tests pinning the blanked Other rate (with the mirrorable rates asserted as still moving, so it is a targeted blank rather than a dead grid) and the invalid-entry freeze. Schedule 5 sub-pages: FIVE new page tests, where there were none. The review proved both mirrors were deletable with 56/56 green, because the unit tests exercise `deriveSubPageTotals` in isolation and never observe the page calling it. Mutation-verified: disabling both mirrors now fails 2 of them. They cover the served-footer reproduction, typing-vs-blur on both the footer and the row rate, the ACCESS single-volume footer against a CAMP-shaped one, the unusable-entry freeze, and read-only rendering a deliberately skewed totals block untouched. Verified: schedule5 216/216, schedule5SubPage 61/61, eslint clean, tsc no errors in src/. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…yed summary (#291) Group 4 of the batch-2 code review fixes. * THE PAGE NO LONGER CONTRADICTS ITSELF MID-TYPING (ruled by Scho). The row Crown derived from the live values per keystroke while the new Totals footer moved on blur, so typing 1000 showed row Crowns of 700 and 400 under a Subtotal Crown of 900 — the arithmetic visibly failing on screen, a worse impression than the stale footer this defect set out to fix. Legacy drove both from ONE handler (`update="otherCrownTabel footerValues"`, schedule3SubtotalOtherCosts.xhtml:74, 83), so every derived cell on the page now settles on the same event. The pre-existing test that pinned the per-keystroke timing is UPDATED rather than worked around, and now asserts both halves: nothing at keystroke, the new Crown at blur. Its premise changed; the behaviour it protects did not. * AN IN-FLIGHT ADDED ROW NOW COUNTS TOWARD THE FOOTER (proven by probe). The mirror fell back to `{}` for a row `handleAdd` appends before its PUT lands, so on a slow link the footer visibly omitted a row sitting in the grid — and did so permanently if the PUT failed validation or errored. It now falls back to the row's LIVE values, which is a far better default than a fabricated zero. Pinned by a new test with a delayed PUT. * THE SUMMARY CONTRACT IS KEYED, NOT POSITIONAL. `mirroredSummary[index]` indexed a returned array against `config.summaryItems` with `noUncheckedIndexedAccess` off, so a short or reordered array would mis-pair figures with labels — or blank a cell — with no type error and no failing test. That is the same invisible-to-tsc shape as the inert-mirror bug this file already memorialises in a comment. `Schedule3SubPageSummaryItem` now carries a `key` and the mirror returns a record. * THE FOOTER MIRROR MOVED OUT OF THE PAGE CONFIG into `schedule3OtherAcceptableCosts/derived.ts`, with its own unit test. AD-5's amendment confines mirror arithmetic to one `derived.ts` per schedule, and as a config lambda it was the only new figure in the batch pinned by nothing. The module records why it uses `sumAsZero` and NOT `sumN`: the service seeds both accumulators at 0 with `nullToZero`, so an empty page shows 0/0/0 rather than three blanks — the opposite of the Schedule 5 and 7A totals, which is exactly the per-figure distinction the two helpers exist to keep straight. * The Included Unacceptable page deliberately supplies NO `deriveSummary`, with a comment saying why: its legacy grid handlers carry no derived render target, so leaving that footer to the Save echo is faithful. One shared component, two different legacy behaviours, and the config decides. RECOVERY NOTE: another agent switched this shared working tree to `pr-9` mid-change. All committed work was safe on the branch; the uncommitted group-4 edits were recovered from the carried-over working tree, except this test file, which came back as pr-9's version and silently dropped the four footer tests committed in 1c39eac (-68 lines). It was restored from HEAD and the two intended edits re-applied — verified by count (6 `footerCells` references, 16 tests). Verified: all Schedule 3 surfaces 76/76, eslint clean, tsc no errors in src/. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… 6 re-mask (#291) Group 5 of the batch-2 code review fixes — the verification and documentation patches. TRANSCRIPTION HONESTY * The Schedule 5 sub-page tests are re-expressed on `Schedule5SubPageServiceTest`'s OWN figures. The header claimed transcription from the backend test; it cited the service SOURCE, while a dedicated backend test already pinned every one of these behaviours with numbers the frontend shared NONE of -- so the two sides pinned the same rules on disjoint values, which is exactly the drift the claim promises to catch. Now: CAMP three rows 10000+2500+500 over 3 x 120,000 -> 13,000 / 0.04; a row's own rate 10000/120000 -> 0.08; ACCESS 7000+3000 over the single 120,000 -> 10,000 / 0.08; and the campSideServesZero / accessSideStaysNull pair, seeded identically at a 60,000 volume so only the service code differs. * The other three headers are NARROWED to the describe blocks they actually cover. A substantial share of their figures is hand arithmetic -- correct, but client-only. Presenting them as server-pinned made the suite look stronger than it is, which is how the earlier inert fix cleared review. * THE SEAM between the two Schedule 5 mirrors is now tested on the CONSUMING side. `Schedule5ServiceTest.campSideAsymmetryYieldsZero` pins that a served Other cost of 0 makes the camp Sub-Total 0, not blank. The sub-page mirror was tested to PRODUCE that zero; the camp mirror was never tested to consume it, and `sumN` would otherwise be reached with all-null and return blank where the server returns 0. FOUR HOLLOW ASSERTIONS REPLACED `Number.isFinite(grandTotal ?? 0)` passed on null AND on any finite value -- now asserts the value (3000). `.not.toBe` between two mirror outputs passed if either were null -- now asserts both (0.15 and 0.30). The Schedule 6 footer was compared to a snapshot of itself, which passed if the footer rendered nothing -- now also asserts the literal served total. Each read as coverage of a specific hazard while admitting the failure it was placed to exclude. sumN COVERAGE The batch's one new shared primitive had no describe block while `addN`, `subN` and `sumAsZero` all do. Added, including the contrast asserted side by side: `sumN(null, null)` is null where `sumAsZero(null, null)` is 0 -- the per-figure distinction the two helpers exist for. SCHEDULE 6 RE-MASK Legacy's handlers re-rendered the FIELD alongside the rate (`render="vol cal …"` / `render="cos cal …"`), re-applying the converter mask. This was the only page whose blur moved a derived cell and left `50000` unmasked beside it. NOTE the regroup must run AFTER the validity gate: putting it first replaced the form mid-validation and stopped the commit landing at all -- caught by the four #291 tests immediately. PROSE DRIFT Four comments and test titles that asserted the opposite of the code they sat on: schedule3SubPage's "last-saved figures, refreshed after Save", Schedule5SubPage.test's "Nothing is summed on the client", Schedule5.test's "never recomputed" title, and schedule5SubPage/index's "Nothing is computed here" (fixed in group 3). A reader would have taken the comment as the contract and "fixed" the code back toward it. Verified: eslint clean, tsc no errors in src/; schedule6 98/98, schedule5 + sub-page + schedule3 surfaces all green. Full suite and build running. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
SScholefield
left a comment
There was a problem hiding this comment.
Overall: this is in very good shape. The exact-decimal Dec core is the right call — the four failure modes documented on derivedMath.ts (toFixed pre-rounding, exponential notation ≥1e21, MAX_SAFE_INTEGER on the scale-10 path, ×10ⁿ losing exact halves) are all real and all now avoided, and the scale-15-before-multiply step in scalingPopOf is exactly the kind of thing that gets "simplified" away later, so thank you for pinning it with a worked example. Keeping perUnitLegacy and perUnitOf as two named primitives instead of one parameterised helper is the right trade: the comment saying "do not unify without changing the backend first" is the whole story in one line. sumN vs sumAsZero as a per-figure choice, transcribed from the service rather than picked per page, is the correct model, and the page-level tests asserting against the echo's own derived fields rather than a pre-Save render snapshot is the right fix for the tautology the earlier review caught.
Three things on the Schedule 3 sub-page, which is the one surface that didn't get the same treatment as the rest.
- The row cells and the footer parse the same text with two different parsers
schedule3SubPage/index.tsx:166 — numeric() still uses lax toNum, while deriveOtherAcceptableSubtotal uses strict committedNum (parseDecimalInput). So for the forms toNum accepts and parseDecimalInput rejects, one cell disagrees with the other:
- Enter 1e3 in a Total $ field. validateOtherAcceptable → Number('1e3') = 1000, in range, no error. Blur.
- Row Crown $ renders 1,000 − pop (via toNum).
- Footer Subtotal Total $ excludes the row entirely (committedNum('1e3') → null → wholeDollars(null) → null → sumAsZero counts it as 0).
Same for 0x10 (→ 16) and mis-grouped 12,34 (→ 1234). That's precisely the "row Crowns of 700 and 400 under a Subtotal Crown of 900" self-contradiction the comment above rowCells says was ruled out — it's just been moved from a timing problem to a parser problem. numeric() wants committedNum, or the derived columns and the footer want to share one parse.
(Sidebar: numeric() also has no finiteness guard, so Infinity renders ∞ in Crown. Pre-existing, but the footer now sits next to it showing 0.)
- The sub-page commits invalid entries; every other surface holds
useCommittedValues documents the rule — an invalid or unusable entry holds its previous committed value, because legacy's failed round-trip left the last valid figures on screen — and Schedules 1/2/3/5/5-sub/6/7A all pass invalid, with Schedule 4 reimplementing the same guard at schedule4/index.tsx:3023. The sub-page's blur commits unconditionally (schedule3SubPage/index.tsx:224-230): no rowErrors check, and it spreads the whole row.values, so an out-of-range 999,999,999 in Total $ moves the footer to a figure the server can never produce while the field sits red beside it. errs is already in scope in rowCells.
- Stale doc comment on deriveSummary
schedule3SubPage/index.tsx:103 still says "return one figure per summaryItems entry, in the same order" — but the contract was deliberately changed to keyed lookup (Record<string, number | null>, keyed off summaryItems[].key) for the reason recorded three lines up. Worth fixing so nobody implements the next deriveSummary positionally.
) CI surfaced 16 Vitest unhandled errors, all one root cause: `commitRate` called `groupInput` without importing it, so every blur threw a ReferenceError. The throw landed AFTER `apply(rateInputsOf(form))`, so the rate itself updated and the defect-291 fix looked correct in manual testing — only the re-mask was lost, leaving `50000` unmasked beside a freshly formatted rate. Nothing caught it. `tsc --noEmit` aborts on this repo's tsconfig (TS5107/TS6310) before reading a single source file; eslint has no-undef off for TS, as standard; and vite build uses esbuild, which does not type-check. An unhandled error in a React event handler fails no test, so the suite stayed green. Three failure-path assertions expected the unmasked value — they had encoded the broken behaviour. Grouping on blur is the established convention here (schedule9, schedule1OtherCosts, CommaNumberInput all assert it), so the expectations were stale, not the fix. Also adds a test that asserts the FIELD, not just the rate — mutation-verified to fail with the original ReferenceError when the import is removed. Separately, OtherAcceptableSubtotal becomes a type alias: Schedule3SubPage's deriveSummary prop is `=> Readonly<Record<string, number | null>>` and TypeScript grants an implicit index signature to a type alias but never to an interface. That was a second error the broken type-check hid. Verified against an origin/main baseline: zero type-error regressions in any file this branch touches. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
schedule1/index.tsx L414/L422 — two three-deep nested ternaries extracted into named helpers with early returns. Sonar is right that these were confusing, and in a specific way: they hid that `derived` has the OPPOSITE precedence in the two figures. For cost, the mirror changes exactly one of three branches (only 140); for the rate, it supersedes every row including 139 and 140. Same two codes, opposite answer — legitimately, because deriveSchedule1 computes a rate for 139 (its volume is user-entered) but no cost for it (the cost is a Schedule 3 pull). Now stated in a comment instead of implied by nesting depth. That refactor turned up a coverage gap: inverting the rate precedence left all 268 schedule1 tests green, so the rule was pinned by nothing. Added a test that asserts 139's rate moves with an entered volume while its cost stays the Schedule 3 pull — mutation-verified, it fails on the inversion. schedule5SubPage/derived.ts L71 — `stampedVolume ?? null` becomes a defaulted `volume` parameter. Equivalent: a default fires only on `undefined`, and every call site passing an explicit `null` already wanted `null`. No behaviour change intended in any of the three. Suite 1291/1293 (two timeout flakes, both pass in isolation), zero unhandled errors, build clean, and zero type errors in the touched files against an origin/main baseline. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…mmit, keyed contract All three findings confirmed against the code; the reviewer's analysis of the parser split was exactly right (toNum yields 1000/16/1234 for 1e3/0x10/12,34 where committedNum yields null). 1. The row cells and the footer now share one parse. `numeric()` moves from lax `toNum` to strict `committedNum`, so a derived Crown cannot disagree with the Subtotal Crown above it. Also closes the sidebar: `toNum` had no finiteness guard, so Infinity rendered as ∞. 2. The blur commit now holds invalid and unusable entries, as every other surface does. One correction to the review here: gating on `errs` would not have worked. `rowErrors` is populated only by `persist` (useEditableCostRows.ts:204) — i.e. on a Save attempt — so it is still empty during the entry the gate has to catch, and the field is not marked invalid until Save either. The gate runs `config.validate` on the blurred value instead. 3. The `deriveSummary` doc comment now states the keyed contract and points at the note in schedule3OtherAcceptableCosts/derived.ts that explains why. Three tests added, each mutation-verified against the specific fix it covers. Finding 1 needed the in-flight-add path to be observable at all: once the commit gate is in place, a lax-only value can never reach `committed`, so the only surface where the two parsers still meet is a locally added row, which falls back to its live values while its PUT is in flight — and `1e3` clears validation, so it can be added. Also dropped a fourth test I had written for "one field's blur commits its neighbours' half-typed entries": that is unreachable, because moving focus to another field blurs the first one. The `prev[row.key]` merge is kept as a correctness tidy, not a bug fix, and is commented as such. Suite 1294/1296 (two timeout flakes, 221/221 in isolation), zero unhandled errors, build clean, no new type errors. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Thanks — all three were real, and all three are fixed in 1116d4d. Your parser analysis was exact; I reproduced it before touching anything:
1. One parser. 2. The commit gate — one correction to the suggestion. Gating on Related: this page doesn't mark a field invalid until Save, so the "field sits red beside it" half of your 999,999,999 scenario doesn't happen here yet — the footer moving to an unproducible figure did, and that's what's fixed. Whether these rows should validate live like the main pages do is a separate question; say the word and I'll open an issue. 3. Keyed contract — doc comment fixed. One note: it now points at the On testing these. Three tests added, each mutation-verified against the fix it covers. Finding 1 was the interesting one: with the gate from finding 2 in place, a lax-only value can never reach I also wrote and then deleted a fourth test, for "one field's blur commits its neighbours' half-typed entries". That's unreachable: moving focus to another field blurs the first one, so a neighbour is always committed by the time this one blurs. I kept the Out of scope, flagged. Suite 1294/1296, zero unhandled errors, build clean. The two failures are the load-timeout flake — 221/221 in isolation, and it has now landed on four different files across five runs, which is its own argument for the suite-timeout item. |
paulushcgcj
left a comment
There was a problem hiding this comment.
This display-only recalculation mirrors the server's schedule-specific rounding and null semantics across the requested schedules, while the server remains authoritative on save.
I did not find another high- or medium-confidence correctness, security, or scope issue after tracing the blur commits, validation gates, save echoes, derived calculations, and frontend CI execution.
The exact BigInt-backed decimal tests and separate per-unit arithmetic paths give us good protection against JavaScript precision errors and contract drift.
The existing Schedule 3 subpage concerns from the earlier review remain the actionable items on this PR.
| expect(wholeDollars(100.6)).toBe(101) | ||
| expect(wholeDollars(100.5)).toBe(101) | ||
| expect(wholeDollars(-100.5)).toBe(-101) | ||
| expect(wholeDollars(100.4)).toBe(100) |
There was a problem hiding this comment.
The exact-decimal core is a strong choice here. Keeping division and HALF_UP rounding in BigInt-backed Dec values protects us from the usual JavaScript boundary errors, and the separate perUnitOf and perUnitLegacy primitives make the schedule-specific backend contracts clear rather than hiding them behind one risky shared formula.
…h an apostrophe This one error is what actually blocks PR #344: the "Frontend Tests" job runs lint -> format:check -> test:cov and died at lint, so the tests never ran. `eslint --fix` output; prettier prefers double quotes over escaping the apostrophe in the test name added by the #344 review pass. `npm run lint` and `npm run format:check` are both clean now. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Fixes #291.
The defect
Legacy recalculated every dependent read-only figure — subtotals, totals,
$/m³— the moment youtabbed out of a field, via a per-field AJAX
changehandler. The modern app computed those figuresserver-side only and refreshed them from the Save response, so the read-only cells sat stale while
you typed and jumped after Save. Users had no way to sanity-check their entry before committing it.
The fix
A display-only mirror of the server's arithmetic, per surface, that updates on blur.
Nothing here is ever sent on a write and the server remains the sole authority for every stored
figure: the Save echo supersedes the mirror unconditionally. This required an amendment to
architecture spine AD-5 ("computed server-side, never accepted from a client") to make explicit
that the rule governs authority and persistence, not display — recorded in the BMAD PR.
Two building blocks:
utils/derivedMath.ts— shared exact-decimal primitives over aBigIntcore. Rounding musthappen in integer space: scaling by
10ⁿloses exact halves (3075/5000 = 0.615rounded to0.61), andtoFixedboth pre-rounds and emits exponential notation at ≥1e21.hooks/useCommittedValues.ts— the blur-committed snapshot.formtracks keystrokes anddrives the inputs; a second
committedcopy advances only on blur and feeds the mirror, so ahalf-typed number never produces a figure. Re-seeded on document identity change.
Surfaces fixed (10)
Schedules 1, 2, 3, 4, 5, 6, 7A, plus three sub-page surfaces: Schedule 5 Camp/Access,
Schedule 3 Other Acceptable Costs, and the Schedule 5 sub-page. One
derived.tspersurface — AD-5's amendment confines mirror arithmetic to exactly one module per surface.
The ticket named Schedules 1–4. An audit of every legacy page carrying both a
changehandler anda derived render target found the same defect on 5, 6 and 7A, and the owner approved widening the
scope. Schedules 8–11 were checked against the legacy source and are genuinely not affected
(Schedule 10 previews from the form; 7B and 9 serve-unless-edited; 8 and 11 have no such handler).
Two things worth knowing if you touch this code
There are two different
$/m³rounding rules, and they disagree. Schedules 1, 3, 5, 6 use thelegacy
CoreUtil.bigDecimalDivision— divide at scale 10 HALF_UP, thensetScale(2). Schedules 2and 4 divide at scale 4. They differ exactly at the 2-decimal display boundary, so the mirror ships
perUnitLegacyandperUnitOfas separate primitives rather than one shared helper. (Also:Schedule1Service.perUnittakes(volume, cost)— reversed from Schedules 2/4.)Null semantics are per figure, not per page.
addNis null only when both operands are null;sumNonly when every operand is null;sumAsZeroyields0for an all-null set;subNisasymmetric. Which one a figure needs comes from the service method it mirrors — the Schedule 3
Other Acceptable footer seeds accumulators at
0(so an empty page shows0/0/0), while theSchedule 5 and 7A totals go blank. Getting this wrong fabricates figures or blanks real ones.
Where a figure could not be mirrored faithfully it was deliberately left served, and the reason is
recorded in the module. The clearest case: Schedule 5's two "Other" rows divide each sub-page row
by the stamped volume and round each term before summing, which is not equal to rounding the summed
cost once — and the camp document carries only the sum. Those rates go blank once the camp volume
moves rather than showing a figure no arithmetic on screen reconciles.
Testing
tscclean, production build clean. The one failure is a pre-existing15s-timeout flake in
Schedule8.test.tsxunder parallel load — that file passes 67/67 inisolation, and Schedule 8 is untouched by this branch. A suite-timeout config change is filed in
the BMAD
deferred-work.md.derivedMathprimitive tests, aderived.test.tsper surface, and page-level testsasserting the rendered cells against the Save echo's server figures — not against the mirror
itself, which is the tautology the first code review caught.
e2e/features/sch2/uc-sch2-001-report-costs/happy-path.feature— DIV-1 closed. The pre-Saveblock now carries the recalculated figures, deliberately identical to the post-Save block, which
is the whole point of the fix.
Review history
Two rounds of
/bmad-code-review(four adversarial layers each) — 14 patches applied, then 25more. Both rounds found real bugs that had shipped behind a green suite, including a Schedule 3
footer mirror that was entirely inert because it gated on a property that does not exist, and a
Schedule 7A commit map that never cleared. Round 2's root cause was systemic: every HIGH finding
was a round-1 lesson applied to Schedules 1–4 and not carried forward to 5/6/7A. That is recorded
in the defect artifact, and the mechanical cross-language link that would prevent it is filed as
deferred work.
Verification
Manually verified against the running app by the issue owner on 2026-08-21 — the ticket's own exit
criterion. All ten surfaces confirmed working.
🤖 Generated with Claude Code
Thanks for the PR!
Deployments, as required, will be available below:
Please create PRs in draft mode. Mark as ready to enable:
After merge, new images are deployed in: